Skip to content

[SYCL] Fix AddSecurityFlags having no side effects #17690

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

AlexeySachkov
Copy link
Contributor

CMake has a notion of scopes for variables and we fell into a pitfall of setting local variables instead of applying changes globally to the whole project.

For reference, see:

CMake has a notion of scopes for variables and we fell into a pitfall of
setting local variables instead of applying changes globally to the
whole project.

For reference, see:
- https://cmake.org/cmake/help/v3.20/command/set.html
- https://cmake.org/cmake/help/v3.20/command/include.html
@@ -31,7 +31,7 @@ macro(add_link_option_ext flag name)
endif()
endmacro()

function(append_common_extra_security_flags)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewers: updating set calls on lines 14-16 to have PARENT_SCOPE may seem like a solution, but there is a problem with append (used on line 28) being a function instead of a macro. append by itself contains PARENT_SCOPE in set it does, but it only propagates results one scope upwards (up to append_common_extra_security_flags), whilst here we need two scopes two scopes up to reach the global scope.

Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@AlexeySachkov
Copy link
Contributor Author

AlexeySachkov commented Mar 31, 2025

Testing on Arc was cancelled (by some unknown to me reason), but I do not expect this patch to have any functional impact and all other testing configurations are green, so I will proceed with merge

@AlexeySachkov AlexeySachkov merged commit 88827d4 into intel:sycl Mar 31, 2025
22 of 23 checks passed
@AlexeySachkov AlexeySachkov deleted the private/asachkov/fix-applying-hardening-flags branch March 31, 2025 10:21
AlexeySachkov added a commit to AlexeySachkov/llvm that referenced this pull request Apr 2, 2025
CMake has a notion of scopes for variables and we fell into a pitfall of
setting local variables instead of applying changes globally to the
whole project.

For reference, see:
- https://cmake.org/cmake/help/v3.20/command/set.html
- https://cmake.org/cmake/help/v3.20/command/include.html
AlexeySachkov added a commit that referenced this pull request Apr 2, 2025
This is a cherry-pick of #17690

---

CMake has a notion of scopes for variables and we fell into a pitfall of
setting local variables instead of applying changes globally to the
whole project.

For reference, see:
- https://cmake.org/cmake/help/v3.20/command/set.html
- https://cmake.org/cmake/help/v3.20/command/include.html
AlexeySachkov added a commit that referenced this pull request Jul 11, 2025
CMake has a notion of scopes for variables and we fell into a pitfall of
setting local variables instead of applying changes globally to the
whole project.

For reference, see:
- https://cmake.org/cmake/help/v3.20/command/set.html
- https://cmake.org/cmake/help/v3.20/command/include.html
AlexeySachkov added a commit that referenced this pull request Jul 16, 2025
This PR contains several cherry-picks and some unique changes which have
not been applied to the `sycl` branch yet.

The intent of this PR is to enable as much (quickly) possible hardening
flags to be in better compliance with our SDL requirements.
The main thing this PR is after are things like immediate bindings,
fortify source, stack protection and `relro`.
The thing that this PR is **not** after are extra warning flags - some
of them we can't apply globally because LLVM itself isn't warning free,
some of them we can't apply even locally to SYCL RT because we haven't
fixed corresponding warnings yet.

Patches which were cherry-picked from the `sycl` branch:

- [SYCL] Fix AddSecurityFlags having no side effects
(#17690)
  - Patch-By: Alexey Sachkov <[email protected]>
- [SYCL] Refresh hardening flags applied to the project
(#18398)
  - Patch-By: Nikita Kornev <[email protected]>
- [SYCL][CMAKE] Refactor -fPIE handling
(#19235)
  - Patch-By: Alexey Sachkov <[email protected]>
- [SYCL][CMAKE] Drop nodlopen from hardening flags
(#19357)
  - Patch-By: Alexey Sachkov <[email protected]>
- [SYCL][CMAKE] Fix _FORTIFY_SOURCE=3
(#19268)
  - Patch-By: Alexey Sachkov <[email protected]>
- [SYCL][CMake] Properly enable -pie hardening flag
(#19447)
  - Patch-By: Alexey Sachkov <[email protected]>

Additional changes which have **not** been applied to the `sycl` branch:
- Adjusted `configure.py` to the new way of `-fPIE` handling
- Dropped `/sdl` flag because LLVM isn't warning-free
  - it will be applied locally to SYCL RT in a separate PR against the `sycl` branch for future releases
- Dropped `/analyze` flag because SYCL RT isn't warning-free 
  - it will be applied locally to SYCL RT in a separate PR against the `sycl` branch for future releases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants